Skip to content

feat: typed callbacks and errors - #17

Merged
loks0n merged 8 commits into
mainfrom
feat/callback-failure
Sep 10, 2026
Merged

loks0n merged 8 commits into
mainfrom
feat/callback-failure

Conversation

@loks0n

@loks0n loks0n commented Sep 10, 2026 •

Copy link
Copy Markdown
Member

Summary

Orchestrator v2.2.0 reports a failed callback's error as {code, message}. This PR gives callbacks a typed surface end to end.

CloudEvent<T> is generic and knows nothing about its payload. CloudEvent::decode($payload, $decode) hands the event type and raw data to a callable and stores whatever comes back; fromArray() is decode() with the identity, so existing raw-array callers are unchanged.

One Callback per event type, dispatched by CallbackEvent::decode(array $data):

Event Callback
orchestrator.job.start JobStart
orchestrator.job.log JobLog
orchestrator.job.artifact JobArtifact — format/compression null when never sniffed, ?Error $error
orchestrator.job.exit JobExit — exitCode, ?reason, ?Error $error
orchestrator.job.complete JobComplete
orchestrator.deployment.response DeploymentResponse — ?statusCode/?body null when the request never reached a replica, ?Error $error

Callback\Error (ErrorCode $code, string $message) with Enum\ErrorCode covering every artifact, exit, and deployment-response code. Strict: a non-object error, unknown code, or missing message throws ClientException. No tolerance for the pre-2.2 string shape — pin the SDK to the orchestrator it talks to.

$event = CloudEvent::decode(
    \json_decode($rawBody, true),
    fn (string $type, array $data) => CallbackEvent::from($type)->decode($data),
);

if ($event->data instanceof JobArtifact && $event->data->error !== null) {
    $log->error("{$event->data->artifactId}: {$event->data->error->message}");
}

Also adds Data::bool for the response callback's truncation flags.

Test plan

  • composer test — each event type decodes to its callback; artifact/exit/response error cases; unknown event type and missing fields rejected; Error object, null-on-success, and malformed shapes
  • composer analyze, composer format:check, composer refactor:check

🤖 Generated with Claude Code

Orchestrator 2.2 reports a failed callback's error as an object: a stable
code to branch on and a message to show. CloudEvent::failure() returns it
as a Failure, accepting the bare-string code a 2.1 orchestrator still
sends mid-upgrade.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The PR is not yet safe to merge because legacy callback errors remain rejected and the callback mapping test still violates the repository’s explicit testing requirement.

Fix All in Claude CodeFindings

  1. P1 Legacy callback errors rejected ▶
  2. P2 Test Copies Production Mapping ▶
Fix with agent prompt
### Issue 1
src/Callback/Error.php:31-33
During a mixed-version rollout, an orchestrator may send a bare error code such as `"job_oom"`. This array-only check throws `ClientException` instead of returning a `Failure` with an empty message, so consumers cannot handle a documented legacy failure.

### Issue 2
tests/Callback/CallbackTest.php:42-47
This provider hard-codes the same six event strings and payload classes defined by `CallbackEvent` and its `decode()` match arms. The test therefore checks a copy of the implementation instead of an independent, observable callback contract, so an internally consistent but wire-incompatible mapping could still pass. This violates the repository directive against implementation-coupled tests and must be corrected before merging.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

  • Makes CloudEvent generic and adds caller-supplied data decoding.
  • Adds typed callback models for job and deployment-response events.
  • Introduces structured Error and ErrorCode contracts.
  • Documents signature-first callback processing and typed dispatch.
  • Renames the earlier payload/failure terminology to callback/error terminology.

Reviews (5) · Last reviewed commit: "fix: callbacks live in Callback, not Cal..."

Comment thread src/Callback/CloudEvent.php Outdated
Comment thread src/Callback/Failure.php Outdated
Comment thread tests/Callback/CloudEventTest.php Outdated
loks0n and others added 4 commits September 10, 2026 11:52
An explicit null error or a non-string message is malformed, not a
success; both now raise ClientException. Tests assert the null result
and the exception type rather than its prose.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Rector's AssertEmptyNullableObjectToAssertInstanceofRector rewrote
assertNull on a ?Failure into assertNotInstanceOf, which hides the
contract the test is for. Skip that rule.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CloudEvent is transport; what a callback's data means is the payload's
business. Failure::fromData($event->data) replaces CloudEvent::failure(),
and Failure's tests move to their own file.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Comment thread src/Callback/Error.php
CloudEvent<T> is an envelope that decodes its data through a callable it
is handed, so it never learns what a callback means. The meaning lives in
Payload: one readonly DTO per event type, each carrying a Failure where
the orchestrator can report one, dispatched by CallbackEvent::decode().

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@loks0n loks0n changed the title feat: expose callback failures as {code, message} feat: typed callback payloads and failures Sep 10, 2026
Comment thread tests/Callback/CallbackTest.php
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@loks0n loks0n changed the title feat: typed callback payloads and failures feat: typed callbacks and errors Sep 10, 2026
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@loks0n
loks0n merged commit 97ae128 into main Sep 10, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant